[WIP] Non-modular plugins with synthetic module #117570
Conversation
…Loader is in the way now)
| description 'The Elasticsearch plugin that powers SQL for Elasticsearch' | ||
| classname = 'org.elasticsearch.xpack.sql.plugin.SqlPlugin' | ||
| extendedPlugins = ['x-pack-ql', 'lang-painless'] | ||
| extendedPlugins = ['x-pack-ql', 'lang-painless', 'x-pack-core'] |
There was a problem hiding this comment.
This change should no longer be needed (I added recurse to parent layers/dependencies for 1 level), I'll revert this.
| @@ -0,0 +1,34 @@ | |||
| module org.elasticsearch.spatial { | |||
There was a problem hiding this comment.
This should go in a separate PR
Also, it probably isn't needed anymore: I did this before I added support for parent layers/dependencies. Now with that, we are able to have a synthetic module referencing another synthetic module (works for esql/esql-core for example)
| try { | ||
| AccessController.doPrivileged((PrivilegedExceptionAction<?>) () -> { | ||
| Class<?> unsafe = Class.forName("sun.misc.Unsafe"); | ||
| // Bypass UberModuleClassLoader, as server does not have our very dangerous permissions |
There was a problem hiding this comment.
This shows an interesting side effect of using UberModuleClassLoader: for class loading, the server module/ProtectionDomain gets in the way. So if it does not have permissions (like accessClassInPackage.sun.misc), no matter if we grant them to the plugin, it won't work. But I don't think this is a problem.
| if (parentLayer != ModuleLayer.boot()) { | ||
| allUnqualifiedExports.addAll(unqualifiedExportsForLayer(parentLayer)); | ||
| allReadableModules.addAll(readableModulesForLayer(parentLayer)); | ||
| // TODO: recurse? |
There was a problem hiding this comment.
We might want to make this fully recursive... maybe?
(right now it does not seem we ever have extensions of extensions, but...)
| return result; | ||
| } | ||
|
|
||
| static String toModuleName2(String name) { |
There was a problem hiding this comment.
Very hacky, but I did not want to break the existing name schema for stable plugins.
And there is a ml-package-loader loader unfortunately -- ml.package.loader is not a OK module name, as package is a reserved keyword.
This reverts commit 241a5b5.
| Map<String, List<String>> allBundledProviders = new HashMap<>(); | ||
| Set<String> servicesUsedInBundle = new HashSet<>(); | ||
| for (Path path : jarPaths) { | ||
| logger.info("Scanning JAR " + path + " for services"); |
There was a problem hiding this comment.
Nit: potentially long strings are often best placed at the end of a log message to make the message easier to read:
logger.info("Scanning JAR for services: {}", path);
|
|
||
| allBundledProviders.compute(serviceName, (k, v) -> createListOrAppend(v, providersInJar)); | ||
| if (providersInJar.isEmpty() == false) { | ||
| logger.info("Adding non-modular providers " + String.join(";", providersInJar)); |
There was a problem hiding this comment.
Nit 2: it's best practice to use {} instead of string formatting; otherwise, you pay the cost of the string formatting operation even when logs are disabled. Since providersInJar is already a list, you can probably just log it as-is:
logger.info("Adding non-modular providers: {}", providersInJar);
There was a problem hiding this comment.
keep in mind this is WIP, very "debugging" PR - these info logs will become debug or be removed altogether, (if they stay, they'll probably become the lambda accepting kind, to avoid computing the String.join if not needed)
| allBundledProviders.compute(serviceName, (k, v) -> createListOrAppend(v, providersInJar)); | ||
| if (providersInJar.isEmpty() == false) { | ||
| logger.info("Adding non-modular providers " + String.join(";", providersInJar)); | ||
| allBundledProviders.compute(serviceName, (k, v) -> createListOrAppend(v, providersInJar)); |
There was a problem hiding this comment.
I know you didn't write it, but this createListOrAppend thing is unnecessary. There's already an idiom for this.
allBundledProviders.computeIfAbsent(serviceName, n -> new ArrayList<>()).addAll(providersInJar);
There was a problem hiding this comment.
Yeah, noticed the same, and there are also some addAll mixed with foreach add; if this ever turns into a real PR I'll fix them.
|
Obsolete - we went in a different direction |
3 participants
No description provided.